Add option to forbid empty test suites #4123

eemeli · 2019-12-08T13:47:06Z

Description of the Change

Adds an option forbidEmptySuite, available in the CLI as --forbid-empty-suite, that will fail a suite with an error Empty suite forbidden if it contains no tests.

Alternate Designs

A similar result could be achieved by an individual customised reporter, but that seems like the wrong layer at which to address this.

Why should this be in core?

Because the other options are also defined in the core.

Benefits

At the moment, if the test definitions fail in a context that does not fail a subsequent mocha.run() (e.g. a browser processing multiple <script> tags), this failure may not be caught by Mocha, and instead it will erroneously display a positive result with "0 passing" tests. This is problematic in particular in CI tests, where the results are often left unchecked if they "pass".

Possible Drawbacks

This adds yet another option.

Applicable issues

Fixes #4062

jsf-clabot · 2019-12-08T13:47:08Z

All committers have signed the CLA.

juergba · 2020-03-11T07:09:23Z

@eemeli I apalogize for not responding to your PR. Are you still interested?
I would like to review and push it through now.

eemeli · 2020-03-11T08:15:01Z

Yes, I'm still interested in this. Happy to have this reviewed and hopefully merged!

juergba · 2020-03-11T12:46:25Z

What about pending tests?

static: it.skip()
conditionally at runtime: before(function() { this.skip(); });

We could end up with zero tests run, but the script would still pass.
If we want to cover above scenario, we would have to check the stats-collector (runner.stats) somehow, at runner's end for the total of tests run.

Some users also create tests dynamically at runtime, when the parsing/creating of the test runner has already been completed.

eemeli · 2020-03-11T16:00:11Z

I would argue that a suite with a skipped test is not "empty", and that the right thing in that case is not to complain. Sure, skipping all tests would therefore result in a suite passing even if none of the tests run, but that'd be intentional in this case.

This logic also allows for using forbidEmptySuite: true together with it.only, which would otherwise fail if there are multiple suites.

eemeli · 2020-03-11T16:19:38Z

Regarding dynamically generated tests, that's an entirely valid point. I don't think the proposed behaviour would be very surprising in that case, but that should probably still be accounted for. Are there other places than before() that might dynamically add tests in an otherwise empty suite? And would the right thing then be to fail before or after the suite-end event is emitted?

juergba · 2020-03-13T08:22:59Z

I don't understand this one:

This logic also allows for using forbidEmptySuite: true together with it.only [...]

With dynamically generated tests I was refering to scenarios like #4126 etc. You can do this in it() and any hook type, but hooks need at least one test in order to run. So the suite wouldn't be empty anyway.

And would the right thing then be to fail before or after the suite-end event is emitted?

At suite-end event, I guess.

In your issue you wrote:

I would like to have an option to consider "0 passing" tests to be considered an error.

In this PR you switched to checking each and every suite, instead of a final test at runner's end. What is your reason?

I'm not sure yet, wether we should go your way, or check once at runner's end.

eemeli · 2020-03-13T10:03:03Z

Perhaps I should rephrase the option to consider it an error if a suite "defines" no tests, rather than "containing" no tests? That might make it altogether clearer.

Regarding it.only, I mean the case of having two parallel suites (say, A and B) each of which defines some tests. If one test in B is marked as it.only, all of the tests in suite A are skipped. With the current formulation, running those test suites with --forbid-empty-suite will pass, but if it's changed to require each suite to contain at least one test that's run, it'll fail.

With dynamically generated tests I was refering to scenarios like #4126 etc. You can do this in it() and any hook type, but hooks need at least one test in order to run. So the suite wouldn't be empty anyway.

Expect for the before() hook, right? As far as I can tell, that's normally run even for an empty suite. So it provides a way for an initially empty suite to end up with some tests. And there's probably all sorts of other places, if you're adding tests to other suites than the current one.

In this PR you switched to checking each and every suite, instead of a final test at runner's end. What is your reason?

Three reasons:

Prefer to fail earlier rather than later
If suite A contains only suite B, which in turn contains only suite C, make sure that we only throw one error.
I presumed that the emptiness of a suite could be determined before it's even run

I'll amend and rebase this PR on top of the latest master, to account for dynamically added tests.

eemeli · 2020-03-13T10:27:33Z

On an entirely different note, I rather appreciate the timing of pointing at an issue filed on the last Friday the 13th, on a Friday the 13th.

juergba · 2020-03-13T12:16:16Z

Yes, and now the corona virus. Will there be any survivors left for using your new feature?

Expect for the before() hook, right? As far as I can tell, that's normally run even for an empty suite.

No, a before() shouldn't run without at least one test.

eemeli · 2020-03-13T15:33:06Z

Well, haven't we all heard recently that testing in particular is rather important just now?

No, a before() shouldn't run without at least one test.

You're right, that was a misunderstanding on my part.

I added a test for a dynamically-defined test in an otherwise empty suite, and it turns out that the current approach already handles that case just like it should.

And then I realised that I can use the already-calculated total from suite.grepTotal to determine if a suite has no tests, and dropped the suite.isEmpty method. This means that the complete change required to runner.runSuite is this if statement.

And therefore I didn't move the test to the suite-end, because the existing behaviour after those lines prevents the suite-begin and suite-end events from being emitted if the suite is empty, and there's no reason to change the existing behaviour.

I also clarified the docs a little bit, and rebased the branch on the current master.

coveralls · 2020-03-13T15:41:23Z

Coverage increased (+0.08%) to 92.886% when pulling a357da0 on eemeli:req-tests into 2f26478 on mochajs:master.

juergba · 2020-03-28T08:24:01Z

@eemeli I will review this weekend.

juergba

@eemeli I don't feel good with this implementation:

I still haven't understood why you switched
- from focus test run: "0 passing" tests to be considered an error
- to focus suite: an empty suite with no tests fails even with a next suite containing tests.

Prefer to fail earlier rather than later: do you want to bail/abort after first empty suite or continue?
The first suite fails (in an incorrect manner), but the test run keeps going. Adding more empty suites, will throw more exceptions. But at the end we have passing tests and a failing test run.

describe("suite", function(){
  describe('inner empty suite', function() { });  
  describe('inner full suite', function() {
    it("test 1", function(){ });
    it("test 2", function(){ });
  });  
});

the total way is not consistent:
- suites with skipped tests only => pass
- suites with tests but not run (grep not matched) => fail

juergba · 2020-03-29T14:11:31Z

lib/runner.js

@@ -732,6 +732,9 @@ Runner.prototype.runSuite = function(suite, fn) {

  debug('run suite %s', suite.fullTitle());

+  if (!total && this.forbidEmptySuite) {
+    this.fail(suite, new Error('Empty suite forbidden'));


Suite does not extend Runnable (as does Test and Hook), so you can't fail a Suite.
The reporter output is weird, if the test file contains only an empty suite.
Maybe a dummy test?

boneskull · 2020-08-18T22:50:20Z

@eemeli Are you still interested in working on this?

boneskull · 2020-08-18T22:54:55Z

FWIW I think this is a reasonable addition. It's not something that can be discovered via static analysis, and people do want to be able to run Mocha w/o this sort of restriction.

At this point though I wonder if we should add something like a "CI mode" which enables various "strict" behaviors...

eemeli · 2020-08-19T09:57:06Z

Sorry, this fell off my radar. Yes, I'm still interested in working on this. I'll need to review @juergba's comments and get back to you on this.

eemeli · 2021-04-04T06:08:28Z

Closing, as I clearly haven't had the time to work on this.

juergba added area: browser browser-specific type: feature enhancement proposal area: node.js command-line-or-Node.js-specific semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Mar 11, 2020

Add option to forbid empty test suites

a357da0

eemeli force-pushed the req-tests branch from c87bcab to a357da0 Compare March 13, 2020 15:30

juergba reviewed Mar 29, 2020

View reviewed changes

eemeli closed this Apr 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to forbid empty test suites #4123

Add option to forbid empty test suites #4123

eemeli commented Dec 8, 2019

jsf-clabot commented Dec 8, 2019 •

edited

Loading

juergba commented Mar 11, 2020

eemeli commented Mar 11, 2020

juergba commented Mar 11, 2020 •

edited

Loading

eemeli commented Mar 11, 2020

eemeli commented Mar 11, 2020

juergba commented Mar 13, 2020

eemeli commented Mar 13, 2020

eemeli commented Mar 13, 2020

juergba commented Mar 13, 2020

eemeli commented Mar 13, 2020

coveralls commented Mar 13, 2020

juergba commented Mar 28, 2020

juergba left a comment

juergba Mar 29, 2020

boneskull commented Aug 18, 2020

boneskull commented Aug 18, 2020

eemeli commented Aug 19, 2020

eemeli commented Apr 4, 2021

Add option to forbid empty test suites #4123

Add option to forbid empty test suites #4123

Conversation

eemeli commented Dec 8, 2019

Description of the Change

Alternate Designs

Why should this be in core?

Benefits

Possible Drawbacks

Applicable issues

jsf-clabot commented Dec 8, 2019 • edited Loading

juergba commented Mar 11, 2020

eemeli commented Mar 11, 2020

juergba commented Mar 11, 2020 • edited Loading

eemeli commented Mar 11, 2020

eemeli commented Mar 11, 2020

juergba commented Mar 13, 2020

eemeli commented Mar 13, 2020

eemeli commented Mar 13, 2020

juergba commented Mar 13, 2020

eemeli commented Mar 13, 2020

coveralls commented Mar 13, 2020

juergba commented Mar 28, 2020

juergba left a comment

Choose a reason for hiding this comment

juergba Mar 29, 2020

Choose a reason for hiding this comment

boneskull commented Aug 18, 2020

boneskull commented Aug 18, 2020

eemeli commented Aug 19, 2020

eemeli commented Apr 4, 2021

jsf-clabot commented Dec 8, 2019 •

edited

Loading

juergba commented Mar 11, 2020 •

edited

Loading